Use gradients and subduing in MultiSeriesBarChart #311

krystalcampioni · 2021-04-30T19:00:51Z

What problem is this PR solving?

Solves: https://github.com/Shopify/core-issues/issues/23793

Implements gradients and updated hovering style subduing inactive bars

⚠️ This branch also introduces the following secondary changes:

SquarePreviewColor can now accept GradientStop[]
Removed duplicated component in favour of components/Bar
Stacked bar chart doesn't break if we pass in a gradient. This change was necessary since both chart types are defined within <MultiSeriesBarChart/>. Even though it works, we should revisit this chart later to check what would be the expected gradient behaviour and introduce animations:

Reviewers’ 🎩 instructions

Before merging

Check your changes on a variety of browsers and devices.
Update the Changelog.
Update relevant documentation.

krystalcampioni · 2021-04-30T19:55:31Z

src/components/Bar/Bar.tsx

@@ -1,17 +1,13 @@
 import React, {useMemo} from 'react';


This is the same component that lived in BarChart/components. I've just moved it up, since all Bars now use the same API

krystalcampioni · 2021-04-30T19:56:10Z

src/components/Bar/tests/Bar.test.tsx

@@ -1,135 +1,95 @@
 import React from 'react';
 import {mount} from '@shopify/react-testing';
 import {scaleBand} from 'd3-scale';


Same as mentioned above, just moving the Bar from BarChart/components up

krystalcampioni · 2021-04-30T19:57:26Z

src/components/Legend/Legend.tsx

+
+            <SquareColorPreview color={color} />
+


Updated this since now SquareColorPreView can handle both solid colors and gradients

src/components/MultiSeriesBarChart/MultiSeriesBarChart.tsx

krystalcampioni · 2021-04-30T19:59:49Z

src/components/MultiSeriesBarChart/components/BarGroup/BarGroup.tsx

-        const handleFocus = () => {
-          onFocus(barGroupIndex);
-        };
+      <mask id={maskId}>


Same strategy used on the BarChart: masking a tag that contains the gradients

krystalcampioni · 2021-04-30T20:13:49Z

src/components/MultiSeriesBarChart/components/StackedBarGroup/StackedBarGroup.tsx

+            <StackMarkup
+              data={data}
+              xScale={xScale}
+              onFocus={onFocus}
+              ariaHidden
+              activeBarId={activeBarId}
+              accessibilityData={accessibilityData}
+              activeBarGroup={activeBarGroup}
+              yScale={yScale}
+              groupIndex={groupIndex}
+            />


Originally I was simply reusing the markup with a <use/> tag, but I had to convert it to a react component so I can aria-hide the duplicate and prevent them from being focusable

src/components/MultiSeriesBarChart/components/BarGroup/BarGroup.tsx

src/components/SquareColorPreview/SquareColorPreview.tsx

pbojinov · 2021-04-30T23:09:02Z

src/components/SquareColorPreview/stories/SquareColorPreview.stories.tsx

+import {SquareColorPreview, SquareColorPreviewProps} from '../../../components';
+
+export default {
+  title: 'SquareColorPreview',


Let's move towards putting the components/subcomponents into their own storybook group.

Suggested change

title: 'SquareColorPreview',

title: 'Components/SquareColorPreview',

This is a top level component though. What would be the benefit of it being separated from the other stories in its own folder? 🤔

I was thinking that the charts would be in one group and all of the other components that make up the charts such as tooltips, legends, etc could be in another group.

pbojinov · 2021-04-30T23:16:15Z

Something I noticed when testing the MultiSeriesBarChart with VoiceOver. When tabbing through the bars and the primary bar value is 0, the screen reader selection will select the whole chart. Not sure what's necessarily expected here so I wanted to share it here:

04-30-15-54-qcjaj-1j1pl.mp4

https://screenshot.click/04-30-15-54-qcjaj-1j1pl.mp4

src/components/MultiSeriesBarChart/types.ts

src/components/MultiSeriesBarChart/MultiSeriesBarChart.tsx

src/components/MultiSeriesBarChart/components/StackedBarGroup/StackedBarGroup.tsx

krystalcampioni · 2021-05-03T15:39:27Z

@pbojinov I don't think that is a problem. The black square that appears around the element tries to highlight visually the element currently selected. If the value is 0 the element is indeed invisible, so it uses the chart as a fallback.

src/components/MultiSeriesBarChart/stories/MultiSeriesBarChart.stories.tsx

...onents/MultiSeriesBarChart/components/StackedBarGroup/components/StackMarkup/StackMarkup.tsx

src/components/MultiSeriesBarChart/components/StackedBarGroup/StackedBarGroup.tsx

carysmills

Code and 🎩 looks good to me
Left a suggestion about the types in StackedBarGroup

Can you please make sure to update MultiSeriesBarChart.md with the changes to the props, as well as the changelog?

krystalcampioni changed the title ~~Remove <Bar> duplication, remove highlight color~~ Use gradients and subduing in MultiSeriesBarChart Apr 30, 2021

krystalcampioni commented Apr 30, 2021

View reviewed changes

src/components/MultiSeriesBarChart/MultiSeriesBarChart.tsx Outdated Show resolved Hide resolved

krystalcampioni commented Apr 30, 2021

View reviewed changes

krystalcampioni requested review from carysmills, pbojinov and susiekims April 30, 2021 20:14

pbojinov reviewed Apr 30, 2021

View reviewed changes

src/components/MultiSeriesBarChart/components/BarGroup/BarGroup.tsx Outdated Show resolved Hide resolved

pbojinov reviewed Apr 30, 2021

View reviewed changes

src/components/MultiSeriesBarChart/components/BarGroup/BarGroup.tsx Outdated Show resolved Hide resolved

pbojinov reviewed Apr 30, 2021

View reviewed changes

src/components/SquareColorPreview/SquareColorPreview.tsx Outdated Show resolved Hide resolved

pbojinov reviewed Apr 30, 2021

View reviewed changes

carysmills reviewed May 3, 2021

View reviewed changes

krystalcampioni commented May 3, 2021

View reviewed changes

src/components/MultiSeriesBarChart/components/StackedBarGroup/StackedBarGroup.tsx Outdated Show resolved Hide resolved

krystalcampioni force-pushed the multiseries_bar_gradients branch from abaf030 to b92fffe Compare May 3, 2021 15:35

susiekims reviewed May 3, 2021

View reviewed changes

src/components/MultiSeriesBarChart/stories/MultiSeriesBarChart.stories.tsx Outdated Show resolved Hide resolved

susiekims reviewed May 3, 2021

View reviewed changes

...onents/MultiSeriesBarChart/components/StackedBarGroup/components/StackMarkup/StackMarkup.tsx Outdated Show resolved Hide resolved

krystalcampioni force-pushed the multiseries_bar_gradients branch 2 times, most recently from 4e5cc07 to 3e2e5d5 Compare May 3, 2021 18:09

pbojinov approved these changes May 3, 2021

View reviewed changes

carysmills reviewed May 4, 2021

View reviewed changes

src/components/MultiSeriesBarChart/components/StackedBarGroup/StackedBarGroup.tsx Show resolved Hide resolved

carysmills approved these changes May 4, 2021

View reviewed changes

krystalcampioni force-pushed the multiseries_bar_gradients branch from 3e2e5d5 to e46d3d9 Compare May 4, 2021 15:52

krystalcampioni added 3 commits May 4, 2021 11:58

Remove <Bar> duplication, remove highlight color

0f244bd

use mask and gradients in grouped bar chart

431a17c

allow gradients and solid colors on stacked bar chart'

02720c3

krystalcampioni added 2 commits May 4, 2021 11:58

prevent focusing on aria hidden items

09c91d5

update tests and types

ad338b6

krystalcampioni force-pushed the multiseries_bar_gradients branch from e674bd0 to ad338b6 Compare May 4, 2021 15:59

krystalcampioni merged commit 17d16e8 into master May 4, 2021

krystalcampioni deleted the multiseries_bar_gradients branch May 4, 2021 16:06

	title: 'SquareColorPreview',
	title: 'Components/SquareColorPreview',

Use gradients and subduing in MultiSeriesBarChart #311

Use gradients and subduing in MultiSeriesBarChart #311

Uh oh!

Conversation

krystalcampioni commented Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem is this PR solving?

Reviewers’ 🎩 instructions

Before merging

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pbojinov commented Apr 30, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

krystalcampioni commented May 3, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carysmills left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

krystalcampioni commented Apr 30, 2021 •

edited

Loading